Retry request when Github API returns status 404 #191
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @rust-highfive (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
This doesn't retry on 404 requests though. Also, I'm not that familiar with this project. r? @davidalber |
There was a problem hiding this comment.
Thank you very much for proposing this change!
Aside from the comments I made inline there are a couple other areas I'd like to note:
- The addition of an external dependency breaks the local development instructions in the README. That's both the "Local Development" section and the "Docker" section.
- This PR does not include any changes to the tests, and the current tests won't exercise the retrying (although they should be making sure this has the intended behavior when we don't have response codes of 400). It would ideally modify or add tests. Did you do any other testing to make sure this works as expected?
| @@ -102,6 +106,7 @@ def api_req(self, method, url, data=None, media_type=None): | |||
| body = f.read() | |||
| return { "header": header, "body": body } | |||
|
|
|||
| @retry(HttpClientError, delay=1, backoff=2, max_delay=3) | |||
There was a problem hiding this comment.
Just so I am clear from the documentation of the retry package, this will allow up to three attempts: the initial attempt, another one second after that fails, and another two seconds after the second attempt fails.
It would likely be easier for others to read if we just used the tries argument and drop max_delay.
| @@ -111,6 +116,8 @@ def set_assignee(self, assignee, owner, repo, issue, user, author, to_mention): | |||
| except urllib2.HTTPError, e: | |||
| if e.code == 201: | |||
| pass | |||
| elif e.code == 400: | |||
There was a problem hiding this comment.
Based on #190, these should all probably be handling for the 404 case, not a 400 case.
| @@ -156,6 +164,8 @@ def is_collaborator(self, commenter, owner, repo): | |||
| except urllib2.HTTPError, e: | |||
| if e.code == 404: | |||
| return False | |||
There was a problem hiding this comment.
If we use retry here, it will change the behavior of this method when all of the attempts fail. It would be nice if retry could return a value we provide on the final failure.
| @@ -9,4 +9,5 @@ | |||
| author_email='[email protected]', | |||
| url='https://github.com/rust-lang-nursery/highfive', | |||
| packages=['highfive'], | |||
| install_requires=['retry'], | |||
There was a problem hiding this comment.
I feel like this project has aimed to avoid adding external dependencies in the past, but that may be in my head. Pinging @nrc, if he has any feelings about this.
|
@davidalber |
related with #190
I don't know that we retry only when we receive status 404.
@pietroalbini Would you review this?